Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not inhibit the dead code elimination. #341

Closed

Conversation

dominiquelefevre
Copy link

Avoid calls to value.TypeString() that do not request fully qualified type names. The implementation of TypeString() depends on getting methods by index which disables the DCE in the compiler.

k8s.io/kube-openapi and k8s.io/apiserver happen to depend on go-cmp outside tests so it is important that go-cmp does not inhibit the DCE inadvertently.

Avoid calls to value.TypeString() that do not request fully qualified
type names. The implementation of TypeString() depends on getting
methods by index which disables the DCE in the compiler.

k8s.io/kube-openapi and k8s.io/apiserver happen to depend on go-cmp
outside tests so it is important that go-cmp does not inhibit the DCE
inadvertently.
@dominiquelefevre
Copy link
Author

Hey, anybody out here? @dsnet could you please look at the patch?

@dsnet
Copy link
Collaborator

dsnet commented Sep 19, 2023

The cmp package is a testing-first package, so precise printing of types is a high priority.

I'm not sure I see how this helps since value.TypeString is still transitively called from cmp.Diff.

@dominiquelefevre
Copy link
Author

k8s.io/kube-openapi does not use cmp.Diff() itself. It happens to depend on cmp.Option which brings the following dependency chain:

  1. cmp.Option's filter() depends on cmp.state,
  2. cmp.state brings in cmp.Path which brings in cmp.pathStep,
  3. cmp.pathStep's method String() depends on TypeString().

@dsnet
Copy link
Collaborator

dsnet commented Sep 19, 2023

Can you point at concrete code? It's a bit esoteric to me that code would depend only on cmp.Option but not but not the rest of cmp functionality.

@dominiquelefevre
Copy link
Author

GRPC turns out to have an accidental use of go-cmp outside tests, too. It uses cmp.Equal(). The latest current release, 1.58.1, has this: https://github.com/grpc/grpc-go/blob/863de7347326bdca22f6084e3ae28e8fa96d01ca/balancer/grpclb/grpclb_remote_balancer.go#L58

After this PR, GRPC no longer imports Method() via go-cmp.

@neild
Copy link
Collaborator

neild commented Sep 20, 2023

These both seem like cases that are better addressed by the place misusing cmp.

In the case of k8s.io/kube-openapi, I don't see why that package is taking a dependency on cmp just to provide the SwaggerDiffOptions var. There's nothing in that var that a caller can't construct themselves. If the package wants to make the Ref type easily comparable by cmp, it could provide a Ref.Equal method.

In the gRPC case, the use of cmp.Equal to compare two slices of proto.Message is very heavyweight. I'd replace it with a more directed comparison. (Whenever gRPC is happy requiring Go 1.21, slices.EqualFunc would be the simplest replacement.)

@dominiquelefevre
Copy link
Author

These are very reasonable arguments. The use in kube-openapi looks like a bug to me and I've filed them a PR. The use in gRPC, although inefficient, looks legit to me.

What bothers me more is that we are talking about a whack-a-mole solution. You've implemented some really useful features, and people will use them without thinking twice about go-cmp being mostly a package for tests. Kube-openapi, kube-apiserver and gRPC are just examples that were really trivial to find. There certainly are more. I think it is important to minimise the damage that go-cmp causes when used less than prudently. Especially given that disabled DCE seriously pessimises the code generation.

@neild
Copy link
Collaborator

neild commented Sep 20, 2023

The cmp documentation is quite explicit that it's designed for use in tests, and is not suitable for use in production contexts:

It is intended to only be used in tests, as performance is not a goal and it may panic if it cannot compare the values.

That line undersells "performance is not a goal", however. For example, that gRPC use of cmp.Equal will compare each individual list item twice, in separate goroutines, to verify that the Comparer is symmetric, deterministic, and does not modify its parameters.

This is quite useful for catching inadvertent errors in tests, but is really not suitable behavior for any production context.

@dsnet
Copy link
Collaborator

dsnet commented Sep 20, 2023

If there were a build tag to isolate tests, I'd be more open to something like this, but as it stands we're going to prioritize good reporting of types in tests over reduced binary bloat.

@dominiquelefevre dominiquelefevre closed this by deleting the head repository Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants